Add unit tests for osism/services websocket_manager and event_bridge#2460
Conversation
bdab8be to
322dfc7
Compare
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Several tests reach deeply into EventBridge/WebSocketManager internals (e.g. _redis_client, _redis_subscriber, _shutdown_event, private thread fields), which may make future refactoring harder; consider adding small public helpers or using dependency injection to exercise behavior without coupling to private attributes.
- The WebSocketManager broadcaster tests rely on fixed asyncio.sleep durations to assert behavior; using synchronization primitives (e.g. an event set after send_text is awaited or draining the queue) would make these tests more deterministic and less timing‑sensitive.
- The event extraction parametrization in test_broadcast_event_from_notification is quite dense; extracting the payload-building logic into named helpers or smaller focused tests per service type could improve readability and ease future changes to the extraction rules.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Several tests reach deeply into EventBridge/WebSocketManager internals (e.g. _redis_client, _redis_subscriber, _shutdown_event, private thread fields), which may make future refactoring harder; consider adding small public helpers or using dependency injection to exercise behavior without coupling to private attributes.
- The WebSocketManager broadcaster tests rely on fixed asyncio.sleep durations to assert behavior; using synchronization primitives (e.g. an event set after send_text is awaited or draining the queue) would make these tests more deterministic and less timing‑sensitive.
- The event extraction parametrization in test_broadcast_event_from_notification is quite dense; extracting the payload-building logic into named helpers or smaller focused tests per service type could improve readability and ease future changes to the extraction rules.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
322dfc7 to
c855c4a
Compare
84bd3ef to
20dd1b8
Compare
ideaship
left a comment
There was a problem hiding this comment.
This PR needs a rebase.
20dd1b8 to
2e0a7a9
Compare
| subscriber = MagicMock() | ||
| subscriber.subscribe.side_effect = ConnectionError("down") | ||
| bridge._redis_subscriber = subscriber | ||
| init_redis = mocker.patch.object(bridge, "_init_redis") |
There was a problem hiding this comment.
Mocking _init_redis as a no-op means self._redis_subscriber is never replaced during a reconnect, so these tests can't exercise — and would stay green under — a lifecycle bug in the loop. In event_bridge.py, when subscribe() (line 169) raises, the except calls _init_redis() (line 221), which reassigns self._redis_subscriber = self._redis_client.pubsub() (line 69) to a fresh object without closing the failed one. The finally (lines 225–230) then closes self._redis_subscriber — now the fresh replacement — so the failed subscriber leaks and the next iteration attempts to subscribe using the freshly created but already closed subscriber. Whether Redis internally manages to reconnect from that state can depend on the Redis client implementation, but the lifecycle ordering is unquestionably wrong. Could you fix the ordering in production (close the failed subscriber before re-init, or don't close the fresh one) in its own commit, and make the reconnect side effect here install successive distinct subscriber mocks so the test asserts each retry subscribes on the newly created instance? As written the tests only check call counts, which pass either way. (Same applies to test_gives_up_after_max_retries below.)
There was a problem hiding this comment.
Fixed in 9213f1f: the except handler now calls a new _close_subscriber() helper to close the failed subscriber (and drop the reference) before _init_redis() replaces it, and the per-iteration finally is gone — the loop closes the current subscriber exactly once on exit, so the fresh replacement is never closed prematurely. Both reconnect tests (test_subscribe_error_waits_and_reinitializes_redis, test_gives_up_after_max_retries) now install successive distinct subscriber mocks via the _init_redis side effect and assert that each retry subscribes on the newly created instance and that every failed subscriber is closed exactly once.
|
|
||
| def test_broadcasts_event_via_manager(self, bridge): | ||
| manager = MagicMock() | ||
| manager.broadcast_event_from_notification = AsyncMock() |
There was a problem hiding this comment.
Asserting broadcast_event_from_notification is awaited once with the right args is a fine check of _process_single_event's local contract, but the AsyncMock removes the one boundary that's actually broken in production. _process_single_event (event_bridge.py:237) runs in a worker thread and drives the coroutine on a freshly created event loop (lines 247–255), while broadcast_event_from_notification awaits event_queue.put() (websocket_manager.py:138) on an asyncio.Queue (:89) whose broadcaster waiter lives on the API loop (:211). Waking that waiter from the worker thread isn't thread-safe — it raises under asyncio debug mode and, in production, mutates the API loop's ready-queue without waking it, so delivery is unreliable and the error is swallowed at websocket_manager.py:201. Please marshal onto the API loop (e.g. asyncio.run_coroutine_threadsafe) in a separate commit, and add one integration-style test with a real manager + active broadcaster + worker thread. The assertion here stays valid under that fix, so it needn't change — the gap is the missing coverage, not this test.
There was a problem hiding this comment.
Fixed in 64006d6: WebSocketManager.connect() now records the running loop when it starts the broadcaster task (exposed as WebSocketManager.loop), and _process_single_event() submits the coroutine to that loop with asyncio.run_coroutine_threadsafe() and waits on the future. The private-loop path remains only as a fallback while no broadcaster loop exists yet — the queue has no waiters then, so driving it from the worker thread is safe. Added the requested integration-style test (test_worker_thread_event_reaches_broadcaster_loop): real WebSocketManager, active broadcaster on the test loop, _process_single_event() called from a worker thread via asyncio.to_thread, asserting delivery to a connected client. The existing unit tests keep their assertions unchanged and only set manager.loop = None so they keep exercising the fallback path.
|
|
||
| subscriber.get_message.side_effect = get_message | ||
| bridge._redis_subscriber_loop() | ||
| assert subscriber.subscribe.call_count == 2 |
There was a problem hiding this comment.
This asserts subscribe and close are each called twice, i.e. it encodes "a get_message error resubscribes immediately" as the expected recovery — but that recovery is itself broken. On this path the inner loop breaks (event_bridge.py:203–205) and falls straight into the finally (225–230), which closes self._redis_subscriber without calling _init_redis(), so the next outer iteration subscribes on the same, now-closed subscriber (the same lifecycle-ordering issue as the subscribe-failure path, minus the re-init). Layered on top of that is a separate defect: this path never increments retry_count and never waits — the max_retries/retry_delay back-off only guards the outer subscribe exception path (207–217), and a successful resubscribe resets retry_count to 0 (line 171), so a persistently-failing get_message busy-loops forever with no delay. If you fix these in separate commits (recreate the subscriber before resubscribing, and route this path through the same bounded back-off), this test's exact call counts will need to move with the fix — worth updating the assertion at the same time so it documents the hardened behavior rather than the current one.
There was a problem hiding this comment.
Fixed in ab74671 (on top of 9213f1f): the get_message error is now re-raised into the outer handler instead of breaking, so the path runs through the same bounded back-off as a failing subscribe() — the failed subscriber is closed, retry_count is incremented and capped by max_retries, the loop waits retry_delay, and the resubscribe happens on the fresh subscriber from _init_redis(). One deviation from the suggested split: after 9213f1f fixes the except-handler lifecycle, the single re-raise delivers both the subscriber recreation and the bounded back-off, so both defects land in one commit rather than two. The test is renamed to test_get_message_error_reconnects_with_backoff and asserts the back-off wait, the retry accounting and that the resubscribe happens on the newly installed instance.
| def test_full_queue_drops_event_with_warning(self, bridge, mocker, caplog): | ||
| caplog.set_level(logging.WARNING, logger="osism.event_bridge") | ||
| bridge._redis_client = None | ||
| mocker.patch.object(bridge._event_queue, "put_nowait", side_effect=queue.Full) |
There was a problem hiding this comment.
_event_queue is an unbounded queue.Queue() (event_bridge.py:30), so put_nowait never raises queue.Full and the except queue.Full handler (133–134) is unreachable in production — this test only reaches it by patching put_nowait to raise. Fine to keep as a guard, but could you note in the test name/docstring that it covers a defensive branch unreachable with the current unbounded queue, so it isn't read as real drop behavior?
There was a problem hiding this comment.
Done — test_full_queue_drops_event_with_warning now carries a docstring stating it covers a defensive branch that is unreachable with the current unbounded queue.Queue and is only reachable by patching put_nowait, so it isn't read as real drop behavior. Folded into the test commit (79305cb).
64006d6 to
0bcce7b
Compare
ideaship
left a comment
There was a problem hiding this comment.
Code looks good, but needs a rebase with conflict resolution before it can be merged.
Create the tests/unit/services package with test modules for osism/services/websocket_manager.py and osism/services/event_bridge.py, which previously had no unit test coverage. test_websocket_manager.py covers the EventMessage value object, the per-connection filter logic in WebSocketConnection.matches_filters, the WebSocketManager connection registry (connect/disconnect/update_filters), the event queue helpers (add_event/send_heartbeat), the per-service identifier extraction in broadcast_event_from_notification and the _broadcast_events broadcaster loop including disconnect cleanup and cancellation handling. test_event_bridge.py covers Redis initialization including environment variable overrides and connection failures, the publish path in add_event with reconnect and local-queue fallback, thread startup guards in set_websocket_manager, the _redis_subscriber_loop including resubscribe and retry exhaustion, _process_single_event, the _process_events loop and shutdown. The thread loops are called synchronously and terminated via _shutdown_event-driven side effects, so no real threads, sockets or sleeps are involved. Most WebSocketManager methods are coroutines, so pytest-asyncio is added to the Pipfile dev packages (with asyncio_default_fixture_loop_scope set in setup.cfg); async tests are marked with pytest.mark.asyncio explicitly to keep --strict-markers happy. Loop-driving tests carry pytest-timeout markers so a regression cannot hang CI. Assisted-by: Claude:claude-fable-5 Signed-off-by: Christian Berendt <berendt@osism.tech>
When subscribe() raised in _redis_subscriber_loop, the except handler called _init_redis(), which reassigned self._redis_subscriber to a fresh pubsub object without closing the failed one. The per-iteration finally block then closed self._redis_subscriber - by that time the fresh replacement - so the failed subscriber leaked and the next iteration subscribed on an already closed object. Introduce _close_subscriber(), close the failed subscriber in the except handler before _init_redis() replaces it, and replace the per-iteration finally cleanup with a single close when the loop exits, so the freshly created subscriber is never closed prematurely. The reconnect tests now install successive distinct subscriber mocks via the _init_redis side effect and assert that each retry subscribes on the newly created instance and that every failed subscriber is closed exactly once. Assisted-by: Claude:claude-fable-5 Signed-off-by: Christian Berendt <berendt@osism.tech>
A failing get_message() broke the inner loop of _redis_subscriber_loop and immediately resubscribed: the per-iteration cleanup closed the subscriber without recreating it, so the next iteration subscribed on a closed object, and the path bypassed the retry counter and _shutdown_event.wait() back-off entirely, so a persistently failing get_message() busy-looped forever. Re-raise the error instead so it runs through the same bounded back-off as a failing subscribe(): the failed subscriber is closed, retry_count is incremented and capped by max_retries, the loop waits retry_delay seconds, and _init_redis() provides a fresh subscriber before the resubscribe. The resubscribe test now installs a distinct fresh subscriber via the _init_redis side effect and asserts the back-off wait, the bounded retry accounting and that the resubscribe happens on the new instance. Assisted-by: Claude:claude-fable-5 Signed-off-by: Christian Berendt <berendt@osism.tech>
_process_single_event() runs in the event bridge's worker threads and drove broadcast_event_from_notification() on a freshly created event loop per call. The coroutine awaits event_queue.put() on the manager's asyncio.Queue, whose broadcaster waiters live on the API loop; waking them from a worker thread is not thread-safe - it raises under asyncio debug mode and in production mutates the API loop's ready queue without waking it, so delivery was unreliable and the error was swallowed. Capture the running loop in WebSocketManager.connect() when the broadcaster task is started and expose it as WebSocketManager.loop. The event bridge now submits the coroutine to that loop with asyncio.run_coroutine_threadsafe() and waits for the result. The private-loop path remains only as a fallback for when no broadcaster loop exists yet; the queue has no waiters then, so it is safe. Add an integration-style test that runs a real WebSocketManager with an active broadcaster and calls _process_single_event() from a worker thread, asserting the event is delivered to a connected client. Assisted-by: Claude:claude-fable-5 Signed-off-by: Christian Berendt <berendt@osism.tech>
0bcce7b to
46f6c45
Compare
Closes #2357.
Creates the
tests/unit/services/package with unit tests forosism/services/websocket_manager.pyandosism/services/event_bridge.py, which previously had no coverage.Writing and reviewing the tests surfaced three production defects in
osism/services/event_bridge.py; they are fixed in separate commits on top of the test commit.Production fixes
subscribe()raised,_init_redis()replacedself._redis_subscriberwithout closing the failed instance, and the per-iterationfinallythen closed the fresh replacement — the failed subscriber leaked and the next attempt subscribed on an already closed object. The failed subscriber is now closed before re-initialization, and the loop closes the current subscriber exactly once on exit.get_messagefailures through subscriber back-off: a failingget_message()resubscribed immediately on the closed subscriber and bypassed the retry counter and_shutdown_event.wait()back-off entirely, so persistent failures busy-looped forever. The error is now re-raised into the same bounded back-off path as a failingsubscribe(), which closes the failed subscriber and resubscribes on a fresh one from_init_redis()._process_single_event()drovebroadcast_event_from_notification()on a freshly created event loop in a worker thread, waking the broadcaster'sasyncio.Queuewaiters on the API loop in a non-thread-safe way (unreliable delivery, error swallowed).WebSocketManagernow records the loop its broadcaster runs on, and the bridge submits the coroutine withasyncio.run_coroutine_threadsafe(); the private-loop path remains only as a fallback while no broadcaster loop exists yet (the queue has no waiters then, so it is safe).test_websocket_manager.py
EventMessage: constructor attributes, UUID4 id uniqueness, ISO 8601Z-suffixed timestamp, exactto_dict()keys,to_json()round tripWebSocketConnection.matches_filters(): no-filter pass-through, event/node/service filters incl.node_name=Nonerejection,""→unknownservice mapping and AND-combination of filtersWebSocketManager.connect()/disconnect(): accept + registration, broadcaster started only once while running (done()check), restart after completion, no-op disconnect for unknown websocketsupdate_filters(): full and partial updates, clearing via[], unknown websocket no-opadd_event()/send_heartbeat(): queueing, early return without connections, heartbeat event shapebroadcast_event_from_notification(): identifier extraction for baremetal/compute/nova/network/neutron/volume/image/identity payloads incl. fallback keys, missing payload keys, unknown service types, empty event type, payload copy semantics and the swallowed-exception path_broadcast_events(): filtered delivery,WebSocketDisconnectand generic-error connection cleanup, consumption without connections and clean cancellationtest_event_bridge.py
_init_redis(): default connection parameters,REDIS_HOST/REDIS_PORT/REDIS_DBoverrides, ping failure handling and theREDIS_AVAILABLE = Falselocal-queue-only pathset_websocket_manager(): thread startup guards for subscriber and processor threadsadd_event(): publish payload, zero-subscriber warning, reconnect-then-publish, fallback to the local queue when reconnection fails,queue.Full(documented as a defensive branch unreachable with the unbounded queue) and generic error swallowing_redis_subscriber_loop(): missing subscriber, subscribe/get_message flow, message dispatch with and without a WebSocket manager, invalid JSON, processing errors;subscribe()andget_message()failures both run through the bounded back-off — the tests install successive distinct subscriber mocks via the_init_redis()side effect and assert that each failed subscriber is closed and each retry subscribes on the newly created instance; retry exhaustion and swallowedclose()errors_process_single_event(): missing-manager warning,AsyncMockbroadcast dispatch and swallowed coroutine errors, plus an integration-style test that delivers an event from a worker thread through a realWebSocketManagerwith an active broadcaster to a connected client_process_events(): queued event processing withtask_done(), empty-queue continuation, pre-set shutdown exit and error continuationshutdown(): shutdown event, subscriber close incl. error path and conditional thread joinsNotes
WebSocketManager/EventBridgeinstances instead of the module-level singletons;EventBridgeis always constructed withredis.Redispatched._shutdown_event-driven side effects; the asyncio broadcaster tests bound the task with cancellation infinallyand carrypytest-timeoutmarkers, so no test can hang CI.pytest-asyncio1.4.0 is added to the Pipfile[dev-packages](compatible with pytest 9,pytest<10,>=8.4); async tests are marked explicitly with@pytest.mark.asyncio, so--strict-markersstays happy.🤖 Generated with Claude Code